Drastically speed up Monoid[Map[K, V]] instance. #1300
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes a number of problems I introduced with cats-kernel. 1. Map's monoid was not overriding combineAll 2. It was also doing the wrong thing for combine 3. Monoid should have overridden combineAllOption The performance for map's `combine` is 150x faster after this commit, and `combineAll` is 780x faster. Rather than focusing on how good these speed ups are, I want to apologize for how bad the implementations were previously. The new `combine` is competitive with other implementations, and `combineAll` is about x5 faster than the equivalent code using combine. (All figures using before/after numbers for the given benchmark.) Thanks to Adelbert and Travis for noticing this problem. Also thanks to Travis for producing this benchmark! Finally, thanks to Oscar for helping me look at and fix this issue.
|
This fixes #1290. |
|
👍 |
|
👍, thanks! |
| result | ||
| } | ||
|
|
||
| def wrapMutableMap[K, V](m: mutable.Map[K, V]): Map[K, V] = |
There was a problem hiding this comment.
if you remove this, it becomes unusable from the outside. That was the reason for it, I think, to allow using it from say algebra.
There was a problem hiding this comment.
Ah, right! I'll add them back so we can use them in Algebra (and beyond).
Since we made WrappedMutableMap private to the kernel, we need something like this so that Algebra, Spire, Algebird, etc. can all take advantage of this basic pattern.
Current coverage is 90.76% (diff: 100%)@@ master #1300 diff @@
==========================================
Files 234 234
Lines 3377 3380 +3
Methods 3323 3323
Messages 0 0
Branches 52 55 +3
==========================================
+ Hits 3065 3068 +3
Misses 312 312
Partials 0 0
|
|
Two sign-offs, I'm merging. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes a number of problems I introduced with cats-kernel.
The performance for map's
combineis 150x faster after this commit,and
combineAllis 780x faster. Rather than focusing on how goodthese speed ups are, I want to apologize for how bad the
implementations were previously. The new
combineis competitive withother implementations, and
combineAllis about x5 faster than theequivalent code using combine.
(All figures using before/after numbers for the given benchmark.)
Thanks to Adelbert and Travis for noticing this problem. Also thanks
to Travis for producing this benchmark! Finally, thanks to Oscar for
helping me look at and fix this issue.